Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

zts: avoid piping to special devices #11478

Merged
merged 1 commit into from
Jan 19, 2021

Conversation

aerusso
Copy link
Contributor

@aerusso aerusso commented Jan 18, 2021

Motivation and Context

As described in #11445, the kernel interface kernel_{read,write} no longer act on special devices. In the ZTS, zfs send and receive are tested by piping to these devices, leading to spurious failures (for positive tests) and may mask errors (for negative tests).

Description

Until a more permanent mechanism to address this deficiency is developed, clean up the output from the ZTS by avoiding directly piping to or from /dev/null and /dev/zero.

For /dev/zero input, simply use a pipe: cat </dev/zero | .

However, for /dev/null output, the shell semantics for pipe failures means that zfs send error codes will be masked by the successful | cat >/dev/null command execution. In that case, use a temporary file under $TEST_BASE_DIR for output in favor.

How Has This Been Tested?

Ran the ZTS.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Performance enhancement (non-breaking change which improves efficiency)
  • Code cleanup (non-breaking change which makes code smaller or more readable)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
  • Documentation (a change to man pages or other documentation)

Checklist:

@AttilaFueloep
Copy link
Contributor

However, for /dev/null output, the shell semantics for pipe failures means that zfs send error codes will be masked by the successful | cat >/dev/null command execution.

You could use set -o pipefail.

pipefail
        A pipeline will not complete until all components of the pipeline have completed, and the return  value  will
        be the value of the last non-zero command to fail or zero if no command has failed.


# Redaction snapshots not a descendant of tosnap
log_mustnot zfs redact $sendfs@snap2 book $sendfs@snap2
log_must zfs redact $sendfs@snap2 book2 $clone1@snap $clone2@snap
log_must eval "zfs send --redact book2 $sendfs@snap2 >$stream"
log_must zfs redact $sendfs@snap2 book3 $clone1@snap $clone2@snap
log_must eval "zfs send -i $sendfs@snap1 --redact book3 $sendfs@snap2 \
log_must eval "zfs send -i $sendfs@snap1 --redact book3 $sendfs@snap2 |cat \
>/dev/null"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't that be >$TEST_BASE_DIR/devnull" as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes., good catch.

Copy link
Contributor

@AttilaFueloep AttilaFueloep left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise looks good, besides it may be better to use set -o pipefail to avoid the overhead of creating unused files. Are the devnull files deleted after the test run?

As described in openzfs#11445, the kernel interface kernel_{read,write} no
longer act on special devices.  In the ZTS, zfs send and receive are
tested by piping to these devices, leading to spurious failures (for
positive tests) and may mask errors (for negative tests).

Until a more permanent mechanism to address this deficiency is
developed, clean up the output from the ZTS by avoiding directly piping
to or from /dev/null and /dev/zero.

For /dev/zero input, simply use a pipe: `cat </dev/zero |` .

However, for /dev/null output, the shell semantics for pipe failures
means that zfs send error codes will be masked by the successful
`| cat >/dev/null` command execution.  In that case, use a temporary
file under $TEST_BASE_DIR for output in favor.

Signed-off-by: Antonio Russo <[email protected]>
@aerusso aerusso force-pushed the mrs/upstream/zts-devnull branch from d9c0562 to c7fcf04 Compare January 18, 2021 13:46
@aerusso
Copy link
Contributor Author

aerusso commented Jan 18, 2021

I was very tempted to use set -o pipefail, but ultimately did not because I was worried how it may interfere with other semantics in the tests. In particular, the tests call in shellscript from many different places, and I was not confident that I was not going to miss something. I also considered mkfifo $TEST_BASE_DIR/devnull and cat <$TEST_BASE_DIR/devnull >/dev/null &, but didn't want to worry about job control.

Ultimately, this is an ugly hack that should be reverted ASAP.

@AttilaFueloep
Copy link
Contributor

I was very tempted to use set -o pipefail, but ultimately did not because I was worried how it may interfere with other semantics in the tests.

Yes, that's a valid concern. One could bracket the changed parts with set -o pipefail and set +o pipefail, limiting the scope to just that parts. But since

Ultimately, this is an ugly hack that should be reverted ASAP.

which I agree upon, I don't think it matters.

Copy link
Contributor

@behlendorf behlendorf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! This will at least avoid the ZTS failures for now. I've done some investigation on how this can be detected and handled but so far I haven't found a solution I'm really happy with.

@behlendorf behlendorf added the Status: Accepted Ready to integrate (reviewed, tested) label Jan 19, 2021
@behlendorf behlendorf merged commit f8c4d63 into openzfs:master Jan 19, 2021
behlendorf pushed a commit to behlendorf/zfs that referenced this pull request Jan 22, 2021
As described in openzfs#11445, the kernel interface kernel_{read,write} no
longer act on special devices.  In the ZTS, zfs send and receive are
tested by piping to these devices, leading to spurious failures (for
positive tests) and may mask errors (for negative tests).

Until a more permanent mechanism to address this deficiency is
developed, clean up the output from the ZTS by avoiding directly piping
to or from /dev/null and /dev/zero.

For /dev/zero input, simply use a pipe: `cat </dev/zero |` .

However, for /dev/null output, the shell semantics for pipe failures
means that zfs send error codes will be masked by the successful
`| cat >/dev/null` command execution.  In that case, use a temporary
file under $TEST_BASE_DIR for output in favor.

Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Attila Fülöp <[email protected]>
Signed-off-by: Antonio Russo <[email protected]>
Closes openzfs#11478
behlendorf pushed a commit that referenced this pull request Jan 23, 2021
As described in #11445, the kernel interface kernel_{read,write} no
longer act on special devices.  In the ZTS, zfs send and receive are
tested by piping to these devices, leading to spurious failures (for
positive tests) and may mask errors (for negative tests).

Until a more permanent mechanism to address this deficiency is
developed, clean up the output from the ZTS by avoiding directly piping
to or from /dev/null and /dev/zero.

For /dev/zero input, simply use a pipe: `cat </dev/zero |` .

However, for /dev/null output, the shell semantics for pipe failures
means that zfs send error codes will be masked by the successful
`| cat >/dev/null` command execution.  In that case, use a temporary
file under $TEST_BASE_DIR for output in favor.

Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Attila Fülöp <[email protected]>
Signed-off-by: Antonio Russo <[email protected]>
Closes #11478
jsai20 pushed a commit to jsai20/zfs that referenced this pull request Mar 30, 2021
As described in openzfs#11445, the kernel interface kernel_{read,write} no
longer act on special devices.  In the ZTS, zfs send and receive are
tested by piping to these devices, leading to spurious failures (for
positive tests) and may mask errors (for negative tests).

Until a more permanent mechanism to address this deficiency is
developed, clean up the output from the ZTS by avoiding directly piping
to or from /dev/null and /dev/zero.

For /dev/zero input, simply use a pipe: `cat </dev/zero |` .

However, for /dev/null output, the shell semantics for pipe failures
means that zfs send error codes will be masked by the successful
`| cat >/dev/null` command execution.  In that case, use a temporary
file under $TEST_BASE_DIR for output in favor.

Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Attila Fülöp <[email protected]>
Signed-off-by: Antonio Russo <[email protected]>
Closes openzfs#11478
sempervictus pushed a commit to sempervictus/zfs that referenced this pull request May 31, 2021
As described in openzfs#11445, the kernel interface kernel_{read,write} no
longer act on special devices.  In the ZTS, zfs send and receive are
tested by piping to these devices, leading to spurious failures (for
positive tests) and may mask errors (for negative tests).

Until a more permanent mechanism to address this deficiency is
developed, clean up the output from the ZTS by avoiding directly piping
to or from /dev/null and /dev/zero.

For /dev/zero input, simply use a pipe: `cat </dev/zero |` .

However, for /dev/null output, the shell semantics for pipe failures
means that zfs send error codes will be masked by the successful
`| cat >/dev/null` command execution.  In that case, use a temporary
file under $TEST_BASE_DIR for output in favor.

Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Attila Fülöp <[email protected]>
Signed-off-by: Antonio Russo <[email protected]>
Closes openzfs#11478
@aerusso aerusso deleted the mrs/upstream/zts-devnull branch January 7, 2023 02:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Accepted Ready to integrate (reviewed, tested)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants